-
Notifications
You must be signed in to change notification settings - Fork 217
Fix for the compiler path in compile_commands.json file #1138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for the compiler path in compile_commands.json file #1138
Conversation
Test Results 584 files - 34 584 suites - 34 14m 2s ⏱️ - 22m 28s Results for commit fec0b2a. ± Comparison against base commit fd06dca. This pull request removes 1217 and adds 1 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
1151c1e
to
dabaf25
Compare
...eclipse/cdt/managedbuilder/internal/core/jsoncdb/generator/CompilationDatabaseGenerator.java
Outdated
Show resolved
Hide resolved
dabaf25
to
080da79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like roughly the correct solution. Just some cleanup and review the caching.
...eclipse/cdt/managedbuilder/internal/core/jsoncdb/generator/CompilationDatabaseGenerator.java
Outdated
Show resolved
Hide resolved
...eclipse/cdt/managedbuilder/internal/core/jsoncdb/generator/CompilationDatabaseGenerator.java
Outdated
Show resolved
Hide resolved
...eclipse/cdt/managedbuilder/internal/core/jsoncdb/generator/CompilationDatabaseGenerator.java
Outdated
Show resolved
Hide resolved
080da79
to
d448a14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works well in the normal cases, but there are some corner cases that aren't handled well, see the individual comments.
...eclipse/cdt/managedbuilder/internal/core/jsoncdb/generator/CompilationDatabaseGenerator.java
Show resolved
Hide resolved
...eclipse/cdt/managedbuilder/internal/core/jsoncdb/generator/CompilationDatabaseGenerator.java
Outdated
Show resolved
Hide resolved
d448a14
to
2c4aa3f
Compare
...eclipse/cdt/managedbuilder/internal/core/jsoncdb/generator/CompilationDatabaseGenerator.java
Outdated
Show resolved
Hide resolved
...eclipse/cdt/managedbuilder/internal/core/jsoncdb/generator/CompilationDatabaseGenerator.java
Outdated
Show resolved
Hide resolved
...eclipse/cdt/managedbuilder/internal/core/jsoncdb/generator/CompilationDatabaseGenerator.java
Show resolved
Hide resolved
2c4aa3f
to
84316a4
Compare
d01e65c
to
70bd427
Compare
@alicetrifu Is this PR still work in progress? |
Hello @ghentschke , This should be done. I see that the logs are not available anymore for this, but the comments should be fixed. I see that meanwhile this got into conflict. Should I fix this? |
That would be great. I think we should fix this, since another user has stumbled around this issue. See eclipse-cdt/cdt-lsp#531 (comment)
Yes |
@ghentschke, I have fixed the conflict directly form the Github UI. I hope this is fine. Will wait for the final result of the checks. Thank you! |
There is still a failed unit test. I am not sure if it relates to this PR. |
@ghentschke , This does not seem to be related to my PR indeed. But I am not sure why I have the problem with the Licence. I had this in the past as well and @Kummallinen rerun this for me. I think there are some issues with my permissions. Any idea on what I should do next to fix this ? Thank you! |
I think there is a general problem with the license server at the moment. See eclipse-cdt/cdt-lsp#535 (comment) |
Thamk you @ghentschke! Do I need to make any additional changes here? Not sure how to fix the failing test since I am sure it's not releated to this PR. Please let me know what you think about the status here. Thank you! |
@jonahgraham Do you have any objections here? |
Hi @alicetrifu ,
and this is the compile_commands.json line: "command": "\"C:/msys64/mingw64/bin/x86_64-w64-mingw32-g++.exe\" -O2 -g -Wall -c -fmessage-length\u003d0 -o /MyMakefileProject.o C:/Users/user/cdt/workspaces/cdt-ws_new/MyMakefileProject/MyMakefileProject.cpp", I've seen that the |
Compiler path was wrongly created when anything was set on the "Prefix" option from "Cross Settings". The code needs to check if anything is set there before creating the default path for the compiler.
cfea01e
to
fec0b2a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-based on main and re-approved. AFAICT this can be merged once we get a clean build.
The = will be not converted correctly as the unicode escape string is used \u003d instead.
If this is a problem please raise a new issue/PR for it. If it is an issue it is a sign that the method of escaping used in the writing of compile_commands.json. I suspect it can be resolved by disabling HTML escaping in the GsonBuilder we are using here:
Line 134 in 7420b12
Gson gson = new GsonBuilder().setPrettyPrinting().create(); |
I've seen that the -MMD -MP commands are missing. I am not sure if that's relevant.
MMD/MP are dependency generating commands (the ones that create the extra makedfiles) (docs)
I think these are properly excluded. The CompilationDatabaseGenerator is implemented based on the GnuMakefileGenerator and excluding addDependencyOptions seems correct to me.
I've to check that |
Compiler path was wrongly created when anything was set on the "Prefix" option from "Cross Settings". The code needs to check if anything is set there before creating the default path for the compiler.